-
Notifications
You must be signed in to change notification settings - Fork 179
1.0 compatibility for share operator #369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
1.0 compatibility for share operator #369
Conversation
As someone who had quite some issues with Swift stdlibs version constraints (Apple really has a habit of not caring at all, unfortunately), from my experience, packages like this solve this issue quite well: https://github.com/swhitty/swift-mutex It's comparable to the backport you mentioned in your Pull Request, but more mature. |
@Akazm I think it's the same (I found it in the one you mentioned at the bottom of the readme, the author is the same). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, would be really useful to have share
available for a wide range of OS versions!
Here's a proposed fix for the withLock
crash, and a few indent corrections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More indentation corrections. 🤔
We'd avoid these if the repo had an .editorconfig
file something like:
root = true
[*]
end_of_line = lf
indent_size = 2
indent_style = space
insert_final_newline = true
Co-authored-by: Pyry Jahkola <[email protected]>
Co-authored-by: Pyry Jahkola <[email protected]>
Co-authored-by: Pyry Jahkola <[email protected]>
Co-authored-by: Pyry Jahkola <[email protected]>
Co-authored-by: Pyry Jahkola <[email protected]>
thanks to @pyrtsa the branch is now passing tests properly on my end. |
Co-authored-by: Pyry Jahkola <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the only thing I wonder is whether we can keep typed throws
interface also included under @available(macOS 15.0, iOS 18.0, watchOS 11.0, tvOS 18.0, visionOS 2.0, *)
?
There are a few stored properties making it a bit tricky, but I don't yet think it's impossible.
Just as a heads up, I haven't brushed over this - the implications here are pretty steep so this needs to be carefully considered. Backporting has some severe implications not just to the issues that already are addressed but to the process this package takes beyond just it being hosted here. long/short please have patience with me while I check on a ton of other stuff (and note - those external requirements may have fallout to the proposed alternative here). |
One thought that might have a better pan-out is leaving this as 1.1 but then altering 1.1 to have an earlier OS requirement and then posing a 1.2 for other algorithms. |
Having this You would probably need some rethrows trickery to get this working. If this doesn't work I feel like the pitch needs to be updated as well and point that out. |
@stefanomondino per the general concept of back porting share; Here are the requirements to satisfy -
As I discuss more with the parties this potentially impacts this list may grow a bit - but that is a first blush at what would be required to do what you want. @ph1ps I claim that any destruction of the Generally speaking for anyone looking in the future for back ports of things - the bar is REALLY high in pretty much all maintained packages so this should NOT be viewed as a precedent to justify other back ports - they each have to be considered in their own merit, impact, and fallout (it really does pose a lot of complications for maintaining and delivering packages that are blessed as part of the Apple open source side of things and likewise for the Swift core libraries and even more so for the Swift compiler set of targets). |
Additional tasks that would be extra bonus round stuff:
These are not hard requirements for the author to specifically do but anyone who can pitch in that will definitely make my life easier ;) |
Hello, I've tried to update this concept with your suggestions (I completely missed the "forced throw" thing, thank you for catching it). I've been diving into the matter of this "backportable Also, I've set 1.1 to older iOS versions so that we could run tests in both scenarios (i've tried iOS 17.5 and macos 15, they are both working). Any feedback is welcomed. Thanks. |
Also, on top of that, I don't think we are going to need anything additional documentation for older versions, because it will work as expected - if the underlying sequence never throws, the |
I think we can conditionally make it support typed throws, not just |
I think I've found a way as you said, but the iOS 17 crash is back. Will try again later |
- attempt to throw errors properly
"Hic sunt leones" - at least for me. If I move the next() method inside 1.1 everything works as expected but we lose the typed error on 1.2 (all errors becomes |
fixes: #367
As discussed in the issue with @FranzBusch, this is an attempt to allow the new
share
operator to be available on older Apple platforms (before iOS 18.0 and similar), also known as 1.0 compatibility.This comes with a couple of tradeoffs
Mutex
had to be replaced byManagedCriticalState
, as it's not available on 1.0Failure
generic type for errors had to be replaced with a non-genericSwift.Error
type..milliseconds(10)
definitions (iOS 15.0 min compatibility), and I had to switch to nanoseconds. Using older iOS versions was the quickest thing to do on my end to test this scenario.When ran on iOS 17.5 unfortunatelyManagedCriticalState.withLock
is crashing for some unknown-to-me reason - I know @phausler already encountered this and I didn't fully understand if there's a possible alternative solution / workaround or not.I'm opening this here as a starting point.
Also, I was thinking if something this backport of Mutex could help, in terms of approach:
This is only iOS 16.0+ compatible (not 13.0) but this would be a huge step forward for us developers, since it's way easier to have business direction to accept 16.0 as minimum support, compared to 18.0.
This would probably require the entire library to rename 1.1 into 1.2, having 1.1 to be "intermediately" compatible with iOS 16.0 and 1.2 starting from 18.0 - I perfectly understand this is a huge change but I really think that including share (at least this single one!) it's critical for this package adoption.
UPDATE:
Requirements are
Additional tasks that would be extra bonus round stuff: